Refactor #line processing - keep the original positions in the AST #18699

Martin521 · 2025-06-18T12:17:34Z

Description

Fixes #18553.
Breaking change.

This PR is non-breaking for two of the three #line use cases mentioned in #18553.
It keeps the original ranges in the AST, stores the #line directives in a table and provides a method range.ApplyLineDirectives(). This method is applied in the following places

construction of runtime diagnostics in pattern match compilation
emitting debug information in quotation translation
creating IL source markers (debug information)
formatting fsc compiler diagnostics (CompilerDiagnostics.FormatDiagnosticLocation)
creating service diagnostics (FSharpDiagnostics.CreateFromException)

This should make sure that all debugging information (in .pdb) and all diagnostics stay unchanged.

The third use case, symbol positions for IDE editors, is more tricky. FSharpSymbol has no range field or property that I could apply the transformation to, but just contains (and makes public) the AST items with their original ranges.
So, I propose we leave it like this and call ApplyLineDirectives in the appropriate places in FSAC / the editors.
This will also enable better line directive support in the editors (for some use cases you need the original ranges).
For Ionide / FSAC I found that calling ApplyLineDirectives in a single place (in FSAC's fcsRangeToLspLocation) will most probably be sufficient to support the current functionality.
I don't know what it means for the other editors.
Any thoughts or recommendations here? (@TheAngryByrd @auduchinok @abonie )

List of breaking changes

For fsc and fcs users
- __LINE__ and __SOURCE_FILE__ show original locations now
For fcs users only (tooling)
- AST has original ranges now (ignoring #line directives) (so, also FSharpSymbol points to its original range)

Checklist

Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated

github-actions · 2025-06-18T12:18:13Z

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description

src/Compiler docs/release-notes/.FSharp.Compiler.Service/10.0.100.md

Martin521 · 2025-06-26T06:34:31Z

This is ready for review. See updated PR description.
It introduces no changes for compiler users (except for the hardly used __LINE__ / __SOURCE_FILE__ in combination with #line).
For tooling, see the remarks above.

Martin521 · 2025-07-15T14:11:45Z

@T-Gro
Is there anything I can do to support reviewing this PR?

src/Compiler/SyntaxTree/SyntaxTree.fs

TheAngryByrd · 2025-07-17T14:22:08Z

I expect this to not create any changes to the fsc output. I also assume there are no consumers of the compiler service that rely on having the target ranges in the AST. For them, this would be a breaking change. They would need to apply the map when dealing with these ranges.

Sorry for not getting around to responding sooner. We probably do with FSAC QuickFixes or possibly some analyzers but with all changes to FCS, we adapt. It might be enlightening to see what tests fail with a build of this against FSAC or FSharp.Analyzers.SDK/Ionide-Analyzers/GResearch's analyzers.

Martin521 · 2025-07-17T17:45:04Z

Sorry for not getting around to responding sooner. We probably do with FSAC QuickFixes or possibly some analyzers but with all changes to FCS, we adapt. It might be enlightening to see what tests fail with a build of this against FSAC or FSharp.Analyzers.SDK/Ionide-Analyzers/GResearch's analyzers.

Good to hear. Thanks for looking into it. I will try to run the tests and report back.

Martin521 · 2025-07-20T13:22:41Z

Here is what I found out.

Good news is that goto-symbol is working correctly with the FCS of this PR and the fix in FSAC's fcsRangeToLspLocation that I mentioned in the OP.

I also ran FsAutoComplete.Tests.Lsp

In my environment (fresh dev container), this always (also with the current unchanged FSAC from the repo) ends up hanging in test FSAC.lsp.Ionide WorkspaceLoader.TransparentCompiler.empty file features.tests.type 'c' for checking error and autocompletion starts with 'abs'. So I can only report about the first 5486 tests. Running with the unchanged FSAC, they all pass.
When building FSAC with the FCS from the current dotnet/fsharp repo's main branch and running the tests, I see random failures of one or more of the CodeFix-tests.AdjustConstant tests. I see the same when using the FCS of this PR. No extra failures from the changes of this PR. Which is somehow to be expected because there is no #line in any of the tests.

edgarfgp · 2025-07-30T10:40:46Z

@T-Gro @abonie Can we please get a review here. ?

Copilot

Pull Request Overview

This PR refactors #line directive processing to fix issue #18553. It changes the system to keep original positions in the AST while storing line directives separately, providing a new ApplyLineDirectives() method for when transformed positions are needed.

Key changes:

Original ranges are preserved in the AST, with line directives stored in a separate table
The __LINE__ and __SOURCE_FILE__ intrinsics now show original positions instead of transformed ones
Debug information and diagnostics continue to use transformed positions via ApplyLineDirectives()

Reviewed Changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/fsharpqa/Source/Conformance/SpecialAttributesAndTypes/Imported/CallerInfo/*.fs	Update test expectations to reflect that CallerLineNumber now uses original line numbers
tests/fsharp/core/syntax/test.fsx	Remove extensive line directive tests that no longer apply to the new behavior
tests/ILVerify/*.bsl	Update IL verification baselines to reflect line number changes in generated code
tests/FSharp.Compiler.Service.Tests/ProjectAnalysisTests.fs	Update test to use ApplyLineDirectives() for transformed ranges
tests/FSharp.Compiler.Service.Tests/*.bsl	Add ApplyLineDirectives method to surface area
tests/FSharp.Compiler.ComponentTests/CompilerDirectives/*.fs	Update/add tests for new line directive behavior
src/Compiler/lex.fsl	Replace WarnScopes.RegisterLineDirective with LineDirectiveStore.SaveLineDirective
src/Compiler/Utilities/range.*	Add ApplyLineDirectives method and LineDirectives module for storing directives
src/Compiler/SyntaxTree/WarnScopes.*	Remove line directive tracking and mapping logic
src/Compiler/SyntaxTree/*.fs	Add LineDirectiveStore module and update position handling
src/Compiler/Facilities/prim-lexing.*	Remove OriginalLine and ApplyLineDirective from Position type
src/Compiler/Driver/*.fs	Update to call ApplyLineDirectives for diagnostics and add line directive storage
src/Compiler/Checking/*.fs	Apply line directives for debug info and runtime diagnostics
docs/release-notes/	Document the breaking change

src/Compiler/Utilities/range.fs

Copilot · 2025-07-30T11:12:21Z

src/Compiler/SyntaxTree/WarnScopes.fs

+            OriginalFileIndex: int // TODO: no longer needed
            mutable WarnDirectives: WarnDirective list
-            mutable LineDirectives: LineDirective list
        }

    let private initialData (lexbuf: Lexbuf) =
        {
            OriginalFileIndex = lexbuf.StartPos.FileIndex


The TODO comment indicates dead code that should be cleaned up. Consider removing the unused OriginalFileIndex field if it's truly no longer needed.

Good catch, thanks. I have cleaned it up.

tests/FSharp.Compiler.ComponentTests/CompilerDirectives/Line.fs

T-Gro · 2025-07-30T11:17:55Z

src/Compiler/Driver/CompilerDiagnostics.fs

@@ -2063,6 +2063,7 @@ let FormatDiagnosticLocation (tcConfig: TcConfig) (m: Range) : FormattedDiagnost
            File = ""
        }
    else
+        let m = m.ApplyLineDirectives()


How can we be sure that all the right places have ApplyLineDirectives called?

Could this be solved with a dedicated type, to differentiate between "raw" and "applied" range ?

Do you have a rough proportional estimate on the number of places that need "raw" range vs. the ones that need "applied" range?

I went through all the places that use range.FileIndex or range.FileName, directly or indirectly. (All other uses of range are irrelevant.) If I remember correctly, there were about two or three dozens of them. Of which ten needed the transformation, in the five areas mentioned in the beginning of the description of this PR. The remaining ones are mostly used to create new ranges for the same file, therefore not relevant for the transformation.

I like the idea of a separate type RangeWithLineDirectivesApplied.
BUT

It will need changes in many places (driven by type checking, this might be ok).

I cannot oversee yet how much of the range type and the Range module would need to be duplicated

Can we find a good name?

It will be quite a breaking surface area change (mainly around diagnostics) that will affect many more tools than the current proposal.

Let me know if I should pursue this further.

T-Gro · 2025-07-30T11:20:52Z

src/Compiler/Utilities/range.fsi

@@ -102,6 +102,9 @@ type Range =

    /// The file name for the file of the range
    member FileName: string
+
+    /// Apply the line directives to the range.
+    member ApplyLineDirectives: unit -> range


The signature of this is what I had in mind.
Maybe the input (this) range should be a different type compared to the output ?

See my response above

T-Gro · 2025-07-30T11:23:00Z

src/Compiler/Utilities/range.fs

+    // For use in this module and in range.ApplyLineDirectives only.
+    // The key is the index of the original file. Each line directive is represented
+    // by the line number of the directive and the file index and line number of the target.
+    let mutable store: Map<FileIndex, (int * (FileIndex * int)) list> = Map.empty


How does this work with IDE usage, that is:

Long running (imagine people who do not shut down IDE for weeks)

Handles many projects for a single run of the process

Has to deal with temporal synthetic file occasionally (like file previews etc.)

One aspect are memory leaks, but the data is small.
More important is correctness in the case of multiple projects being parsed - wouldn't their data overwrite each other?

The LineDirectives map is based on and uses the same FileIndex table that maps the file names for all ranges and that ensures unique indices across projects.

In terms of size there is just one small entry per line directive. So most F# projects have no entry at all.

T-Gro · 2025-07-30T11:25:25Z

src/Compiler/Utilities/range.fs

+    // For use in this module and in range.ApplyLineDirectives only.
+    // The key is the index of the original file. Each line directive is represented
+    // by the line number of the directive and the file index and line number of the target.
+    let mutable store: Map<FileIndex, (int * (FileIndex * int)) list> = Map.empty


Is the size O(N) with N= number of line directives scanned?
So a typical F# project will have 0 elements here?

Martin521 added 2 commits June 18, 2025 11:36

refactor #line directive processing

eaa7ebf

Merge remote-tracking branch 'upstream/main' into line-directives

a7c25be

github-project-automation bot added this to F# Compiler and Tooling Jun 18, 2025

github-project-automation bot moved this to New in F# Compiler and Tooling Jun 18, 2025

Martin521 added 7 commits June 18, 2025 12:24

release notes, fantomas

da34d9a

release notes

1201374

Merge remote-tracking branch 'upstream/main' into line-directives

0a2c800

release notes update

61cd61a

ilverify baselines

e301855

update fsharpqa test to reflect new __LINE__ behavior

c3f7f58

test fix

5e37759

Martin521 marked this pull request as ready for review June 26, 2025 08:49

Martin521 requested a review from a team as a code owner June 26, 2025 08:49

Merge remote-tracking branch 'upstream/main' into line-directives

bb905a9

edgarfgp approved these changes Jul 6, 2025

View reviewed changes

Martin521 added 4 commits July 7, 2025 07:45

Merge remote-tracking branch 'upstream/main' into line-directives

602b96f

Merge remote-tracking branch 'upstream/main' into line-directives

6716913

Merge remote-tracking branch 'upstream/main' into line-directives

6c8b218

Merge remote-tracking branch 'upstream/main' into line-directives

26e1e76

nojaf reviewed Jul 17, 2025

View reviewed changes

src/Compiler/SyntaxTree/SyntaxTree.fs Show resolved Hide resolved

Martin521 added 2 commits July 28, 2025 17:28

Merge remote-tracking branch 'upstream/main' into line-directives

8459e56

Merge remote-tracking branch 'upstream/main' into line-directives

bfe3d4d

T-Gro requested a review from Copilot July 30, 2025 11:11

Copilot AI reviewed Jul 30, 2025

View reviewed changes

T-Gro reviewed Jul 30, 2025

View reviewed changes

Martin521 added 6 commits July 30, 2025 13:54

Merge remote-tracking branch 'upstream/main' into line-directives

292d8d0

clean-up WarnScopes.fs

551dbe6

Merge remote-tracking branch 'upstream/main' into line-directives

db2ea63

fantomas

92849c3

Merge remote-tracking branch 'upstream/main' into line-directives

4496b9b

Merge remote-tracking branch 'upstream/main' into line-directives

ba950a3

Martin521 requested a review from T-Gro July 31, 2025 16:17

Refactor #line processing - keep the original positions in the AST #18699

Are you sure you want to change the base?

Refactor #line processing - keep the original positions in the AST #18699

Uh oh!

Conversation

Martin521 commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

github-actions bot commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Release notes required

Uh oh!

Martin521 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin521 commented Jul 15, 2025

Uh oh!

Uh oh!

TheAngryByrd commented Jul 17, 2025

Uh oh!

Martin521 commented Jul 17, 2025

Uh oh!

Martin521 commented Jul 20, 2025

Uh oh!

edgarfgp commented Jul 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Martin521 commented Jun 18, 2025 •

edited

Loading

github-actions bot commented Jun 18, 2025 •

edited

Loading

Martin521 commented Jun 26, 2025 •

edited

Loading